Size Change: +3 B (0%)

Total Size: 1.3 MB

_{compressed-size-action}

Hi @iamthomasbishop I need some design feedback based on the proposal you had shared with me here.

1. Currently in the screenshots above, I did not move the Autoplay component to the bottom of the bottom sheet. Is this something you are still interested in seeing? My rationale for not moving it was, the Settings component for the Video block is shared between both mobile and web so if I moved it, it would also move the location of the component on the web as well and I am not sure if that's a change you would want us to make.
2. In the design spec, you removed the word Note from the help text. eg. Note: Autoplaying audio may cause usability issues for some visitors. became Autoplaying audio may cause usability issues for some visitors. Again, I am utilizing shared logic within the Video block for mobile and web, so if I make this change it would cause a change for both platforms.

@jd-alexander Looking good, thanks for the ping!

did not move the Autoplay component to the bottom of the bottom sheet. Is this something you are still interested in seeing?

I mean "ideally" I'd prefer to see this cell at the bottom because it's the only row with a note attached, but I understand what you're saying and agree it's not worth diverging in this case. So if we're unable to move the cell, I'm fine with shipping this as-is.

Moving forward, it might be worth proposing a more condensed version of the label for all platforms to use, which if accepted, we would simply inherit without additional work. I'm open to any ideas, but here are a few suggestions for a more concise note (which could be used on a variety of blocks that contain autoplay functionality):

• Autoplaying media may cause usability issues
• Autoplay may cause usability issues (also, if we went this route we could afford to keep the Note: part)
• Autoplay may cause usability issues for some users

Let me know if you have any thoughts, but feel free to 🚢 this one and we can proceed if need be with the label changes separately. 👏

I also verified the web version as well for Audio and Video blocks. Neither shows the help text unless the autoplay option is toggled.

@jd-alexander one final comment. I tested on Android with Talkback and the help message never gets read by the screenreader. Maybe the message could be read as part of the accessibilityHint? Just an idea.

I mean "ideally" I'd prefer to see this cell at the bottom because it's the only row with a note attached, but I understand what you're saying and agree it's not worth diverging in this case. So if we're unable to move the cell, I'm fine with shipping this as-is.

No prob @iamthomasbishop I will keep it as is 👍🏾

Moving forward, it might be worth proposing a more condensed version of the label for all platforms to use, which if accepted, we would simply inherit without additional work. I'm open to any ideas, but here are a few suggestions for a more concise note (which could be used on a variety of blocks that contain autoplay functionality):

Autoplay may cause usability issues for some users

I think this is one is a good approach! I will make the necessary updates.

Let me know if you have any thoughts, but feel free to 🚢 this one and we can proceed if need be with the label changes separately. 👏

Will do 🙇🏾

@jd-alexander Looked over the code and tested on Android and iOS and all runs smoothly. I left one code comment for your review -- more of a question for me. I also noticed that the bottom margin below the help text do not match the bottom margins for settings options that do not have help text. I'm wondering if it needs to be slightly increased to give a more uniform looks to settings?

Thanks for spotting this @AmandaRiu working on the changes to make the margins unified.

@jd-alexander one final comment. I tested on Android with Talkback and the help message never gets read by the screenreader. Maybe the message could be read as part of the accessibilityHint? Just an idea.

Good idea! I am working on changing how the accessibility labels are generated. Currently, logic for this lives with the cell and the switch-cell component as well, so I am seeing how I can extend both to support the hint.

1. Hi @AmandaRiu for the margin changes, I realized that Android's cell container has more margin than iOS, so when adjusting the margins I utilized platform-specific styles so the margin is appropriately spaced in relation to the settings option. Grid comparing both platforms below. 330f509

2. For the accessibility changes, I am working on the optimal approach to modifying these two parts of the codebase:

• gutenberg/packages/components/src/mobile/bottom-sheet/cell.native.js

  Line 254 in 330f509

  const getAccessibilityLabel = () => {

• gutenberg/packages/components/src/mobile/bottom-sheet/switch-cell.native.js

  Line 21 in 330f509

  const accessibilityLabel = value

What I am working on accomplishing, is logic that allows the appropriate labels to generate when the help text is available or not without too many duplications of the sprintf templates/logic.

@iamthomasbishop web and mobile now has one string for all platforms 29b4393

Web

Audio Settings	Video Settings

Mobile

Audio Settings	Video

Thanks @ceyhun I am going to be updating the PR with accessibility improvements for @AmandaRiu to review. Thanks for the review 🙇🏾

Hi @AmandaRiu I implemented the accessibility improvements that were requested. Let me know what you think.

Testing

Switch cell

Note

The switch cell implements a different mechanism for generating its accessibility label that includes the toggled state in the label.

Steps

1. Launch the Gutenberg demo app.
2. Enable either Talkback or VoiceOver.
3. Open the Video settings.
4. Notice the Autoplay option reads the help text.
5. Toggle the control and notice that the ON/OFF values changes.

Android	iOS

Cell

Prerequisite

you can add help={ getAutoplayHelp } to SelectControl of the Video Settings. as it utilizes the bottom sheet cell accessibility behavior. All other controls that inherit from cell automatically benefits from this change.

Steps

1. Launch the Gutenberg demo app.
2. Enable either Talkback or VoiceOver.
3. Open the Video settings.
4. Notice the Preload option reads the help text.

Android	iOS

Concerns

The main thing I noticed was that the generated label read by VoiceOver/TalkBack contains an extra full stop at the end because the text has its own full stop and we also include it in the string template being utilized. It seems fine because it doesn't impact the announcement but I am just bringing it up nonetheless.

@jd-alexander LGTM! Nice work!

Thanks for the review @AmandaRiu 😄

@hypest I haven't been able to get this PR merged because I think the current Unit Tests/ PHP are flaky. I have done several re-runs today. The changes here did not touch anything PHP or theme-related so I am thinking that the failure has nothing to do with my changes.

Thanks for the ping Joel! I verified that the failed "Unit tests /PHP (pull_request)" is failing with the same failures on current trunk (commit 29078b0) so, I will go ahead and admin-merge this PR 👍

Why was this merged with failing tests? It's not because it's trunk is failing that that's ok?

Why was this merged with failing tests? It's not because it's trunk is failing that that's ok?

👋 @ellatrix ! No other reason than that actually. It was clear that this PR wasn't introducing the failures and there were no other failures besides those already in trunk. Went ahead with merging to move the native mobile side forward since the particular PHP failures do not block native mobile.

Happy to revise the practice though (merging if PR is dimmed irrelevant to the CI failures) if it's understood that all PRs should block if CI is red.

Also, in this particular case, not sure, has this PR caused some regression perhaps? More than happy to dive in or revert if is has introduced regressions 👍